Deny calls to non-#[const_trait] methods in MIR constck
#132169
Conversation
rustbot
added
S-waiting-on-review
fee1-dead
added
F-effects
|
I'm not totally convinced we should be tweaking const checking here in favor of just reimplementing it the right way from scratch. So I think the right approach would be to 1. check the const conditions of every conditionally const item (including free functions with ~const bound), and eventually delay a bug if they fail (this could for now only be done on CallSource::Normal perhaps) and 2. also make sure that const traits are only callable with the const trait feature enabled. Additionally, the Not totally certain why we're patching the issue partially here if it doesn't necessarily lead us closer to the right approach at the end. I think I expressed that I had been working on a rework of const stability checking that addresses these thoughts, but maybe I didn't make that super clear 😅 |
|
@rustbot author |
rustbot
added
S-waiting-on-author
1 will be achieved through This isn't patching the issue partially because typeck does check const conditions properly, the issue here is that there are no const conditions to check. (because callee is not in a const trait) Denying non-const calls has always been done in MIR constck, so it makes sense to fix denying calls to non-const trait methods here as well. typeck is (atm) only responsible for making sure that const conditions for maybe-const items are satisfied. The separation of responsibilities is pretty clear to me. Therefore if you wanted typeck to take on the role of stability checking and denying non-const calls, that would be orthogonal to this PR. Happy to chat more on Zulip as well. |
|
(and yes, checking stability of concrete impls like that is quite broken as you said, but this PR is about rejecting things we should be rejecting and not fixing the const stability checks) |
There was a problem hiding this comment.
I guess I'll just change the way we do const stability enforcement on top of this. I'd still like to see three things fixed in this PR:
- Rework all of that logic starting with
// Check that all trait bounds that are marked as ~const can be satisfied.-- it's no longer true and no longer doing anything, since~constbounds are not even in thepredicates_ofanymore. Ideally we'd adjust that logic to register and checkconst_conditionsfor all conditionally const items, and keep that as a failsafe for making sure we don't accidentally forget to check~constbounds in HIR, given that many different HIR operations can translate into call terminators during MIR lowering. That's what I meant by (1.) above, not that HIR typeck shouldn't be primarily responsible for enforcing constness. - Remove that call to
Instance::try_resolvecall, since it's really not conceptually well-founded; we shouldn't be specializing constness checking to specific impl items, since this is both inconsistent and unnecessary IMO, and easily side-stepped with things like trivial where clause and indirection via generics. - It would be very nice if you tried to despaghettify the existing logic. There's a lot of nesting of thes
ifstatements and I'm not totally certain that it should exist. @RalfJung had an idea, and this is kinda what I mean by (2.) above, but ideally we'd uplift the const enforcement of trait methods to a newNonConstOpimplementation and enforce const trait calls via that (i.e. override itsstatuswithStatus::Unstable(sym::const_traits)). I'm not sure why we even need to care about theeffectsfeature gate in MIR checking, since IMO that should (at least for now, obviously being removed later) gate just the enforcement of effects in HIR typeck -- MIR should enforce these unconditionally.
|
If you think that's overkill, that's fine, I'd like to hear your thoughts; my justification here is basically that I feel like nothing about fixing rust-lang/project-const-traits#12 is super urgent, and as long as we're touching this const stability checking now, why not clean up the parts that are obviously broken. But if you don't want to do that, then I guess we can probably just land this PR as is, and those other parts can be done later. |
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…=compiler-errors Deny calls to non-`#[const_trait]` methods in MIR constck This is a (potentially temporary) fix that closes off the mismatch in assumptions between MIR constck and typeck which does the const traits checking. Before this PR, MIR constck assumed that typeck correctly handled all calls to trait methods in const contexts if effects is enabled. That is not true because typeck only correctly handles callees that are const. For non-const callees (such as methods in a non-const_trait), typeck had never created an error. /~https://github.com/rust-lang/rust/blob/45089ec19ebebec88bace6ec237244ff0eaa7ad3/compiler/rustc_hir_typeck/src/callee.rs#L876-L877 I called this potentially temporary because the const checks could be moved to HIR entirely. Alongside the recent refactor in const stability checks where that component could be placed would need more discussion. (cc `@compiler-errors` `@RalfJung)` Tests are updated, mainly due to traits not being const in core, so tests that call them correctly error. This fixes rust-lang/project-const-traits#12.
…=compiler-errors Deny calls to non-`#[const_trait]` methods in MIR constck This is a (potentially temporary) fix that closes off the mismatch in assumptions between MIR constck and typeck which does the const traits checking. Before this PR, MIR constck assumed that typeck correctly handled all calls to trait methods in const contexts if effects is enabled. That is not true because typeck only correctly handles callees that are const. For non-const callees (such as methods in a non-const_trait), typeck had never created an error. /~https://github.com/rust-lang/rust/blob/45089ec19ebebec88bace6ec237244ff0eaa7ad3/compiler/rustc_hir_typeck/src/callee.rs#L876-L877 I called this potentially temporary because the const checks could be moved to HIR entirely. Alongside the recent refactor in const stability checks where that component could be placed would need more discussion. (cc ``@compiler-errors`` ``@RalfJung)`` Tests are updated, mainly due to traits not being const in core, so tests that call them correctly error. This fixes rust-lang/project-const-traits#12.
Rollup of 4 pull requests Successful merges: - rust-lang#132124 (coverage: Consolidate creation of covmap/covfun records) - rust-lang#132167 (Replace some LLVMRust wrappers with calls to the LLVM C API) - rust-lang#132169 (Deny calls to non-`#[const_trait]` methods in MIR constck) - rust-lang#132174 (x86 target features: make pclmulqdq imply sse2) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 5 pull requests Successful merges: - rust-lang#132124 (coverage: Consolidate creation of covmap/covfun records) - rust-lang#132140 (Enable LSX feature for LoongArch Linux targets) - rust-lang#132169 (Deny calls to non-`#[const_trait]` methods in MIR constck) - rust-lang#132174 (x86 target features: make pclmulqdq imply sse2) - rust-lang#132180 (Print unsafety of attribute in AST pretty print) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#132169 - fee1-dead-contrib:consttraitsck, r=compiler-errors Deny calls to non-`#[const_trait]` methods in MIR constck This is a (potentially temporary) fix that closes off the mismatch in assumptions between MIR constck and typeck which does the const traits checking. Before this PR, MIR constck assumed that typeck correctly handled all calls to trait methods in const contexts if effects is enabled. That is not true because typeck only correctly handles callees that are const. For non-const callees (such as methods in a non-const_trait), typeck had never created an error. /~https://github.com/rust-lang/rust/blob/45089ec19ebebec88bace6ec237244ff0eaa7ad3/compiler/rustc_hir_typeck/src/callee.rs#L876-L877 I called this potentially temporary because the const checks could be moved to HIR entirely. Alongside the recent refactor in const stability checks where that component could be placed would need more discussion. (cc ```@compiler-errors``` ```@RalfJung)``` Tests are updated, mainly due to traits not being const in core, so tests that call them correctly error. This fixes rust-lang/project-const-traits#12.
This is a (potentially temporary) fix that closes off the mismatch in assumptions between MIR constck and typeck which does the const traits checking. Before this PR, MIR constck assumed that typeck correctly handled all calls to trait methods in const contexts if effects is enabled. That is not true because typeck only correctly handles callees that are const. For non-const callees (such as methods in a non-const_trait), typeck had never created an error.
rust/compiler/rustc_hir_typeck/src/callee.rs
Lines 876 to 877 in 45089ec
I called this potentially temporary because the const checks could be moved to HIR entirely. Alongside the recent refactor in const stability checks where that component could be placed would need more discussion. (cc @compiler-errors @RalfJung)
Tests are updated, mainly due to traits not being const in core, so tests that call them correctly error.
This fixes rust-lang/project-const-traits#12.